Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include ai.operation.parentId from Activity.ParentSpanId instead of Activity.Parent.SpanId #20434

Merged
merged 4 commits into from
Jun 29, 2021
Merged

Include ai.operation.parentId from Activity.ParentSpanId instead of Activity.Parent.SpanId #20434

merged 4 commits into from
Jun 29, 2021

Conversation

mangeg
Copy link
Contributor

@mangeg mangeg commented Apr 15, 2021

Fixes #20433

When starting a new Activity with passing parent context the Parent property of the Activity is null. With the current implementation that means the ParentSpanId is not set on the TelemetryItem.

To maintain the previous functionally the Activity could be created with a ParentContext with no ParentSpanId set.

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

  • Please open PR in Draft mode if it is:
    • Work in progress or not intended to be merged.
    • Encountering multiple pipeline failures and working on fixes.
  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • I have read the contribution guidelines.
  • The pull request does not introduce breaking changes.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

  • Include updated management metadata.
  • Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

  • If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.

  • If the check fails at the Verify Code Generation step, please ensure:

    • Do not modify any code in generated folders.
    • Do not selectively include/remove generated files in the PR.
    • Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
      Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.

    Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:

      `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Apr 15, 2021
@ghost
Copy link

ghost commented Apr 15, 2021

Thank you for your contribution @mangeg! We will review the pull request and get back to you soon.

@ghost ghost added the Community Contribution Community members are working on the issue label Apr 15, 2021
@ghost
Copy link

ghost commented Apr 15, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jun 18, 2021
@ghost
Copy link

ghost commented Jun 18, 2021

Hi @mangeg. Thank you, for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@cijothomas cijothomas mentioned this pull request Jun 22, 2021
Closed
@vishweshbankwar
Copy link
Contributor

Hi @mangeg - Could you please look into conflicts?
I created the same PR since I did not know about this one.
#22061

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Jun 22, 2021
vishweshbankwar
vishweshbankwar reviewed Jun 28, 2021
View reviewed changes
...Monitor.OpenTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.Tests/TagsTests.cs Outdated
@@ -326,7 +326,7 @@ private static Activity CreateTestActivity(IEnumerable<KeyValuePair<string, obje
var eventTimestamp = DateTime.UtcNow;
var traceId = ActivityTraceId.CreateRandom();

var parentSpanId = ActivitySpanId.CreateRandom();
var parentSpanId = default(ActivitySpanId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mangeg - I am not sure why this change is included.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was not supposed to be there. Must have been left in from testing.
Will make a update to revert that change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

cijothomas
cijothomas approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

We may want to add an unit test for the scenario where the parent is in-proc as well, if such a test doesn't exist.

@vishweshbankwar
Copy link
Contributor

LGTM.

We may want to add an unit test for the scenario where the parent is in-proc as well, if such a test doesn't exist.

Good Point - I will get this added in one of the other PRs.

@vishweshbankwar vishweshbankwar merged commit e0623b2 into Azure:main Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishweshbankwar vishweshbankwar vishweshbankwar left review comments

@cijothomas cijothomas cijothomas approved these changes

@rajkumar-rangaraj rajkumar-rangaraj Awaiting requested review from rajkumar-rangaraj rajkumar-rangaraj is a code owner

@reyang reyang Awaiting requested review from reyang reyang is a code owner

@TimothyMothra TimothyMothra Awaiting requested review from TimothyMothra TimothyMothra is a code owner

Assignees
No one assigned
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Milestone
No milestone
4 participants
@mangeg @vishweshbankwar @cijothomas @magnusg-scancloud